Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[1.29] feat/cct-868: Subset of list command options marked as deprecated #3467

Merged

Conversation

grunwmar
Copy link

@grunwmar grunwmar commented Oct 25, 2024

Options

--available 
--all 
--ondate 
--servicelevel 
--no-overlap
--match-install 
--pool-only 
--afterdate

were marked as deprecated
in ListCommand definition of their help string.

The options were marked as deprecated in list command manuald page.

@grunwmar grunwmar force-pushed the feat/CCT-868 branch 2 times, most recently from d76796d to 91d325f Compare October 25, 2024 13:04
@grunwmar grunwmar closed this Oct 25, 2024
@grunwmar grunwmar reopened this Oct 25, 2024
@grunwmar grunwmar changed the base branch from main to subscription-manager-1.29 October 25, 2024 13:10
@grunwmar grunwmar force-pushed the feat/CCT-868 branch 2 times, most recently from 59fdbdc to cc3155f Compare October 25, 2024 13:21
Copy link
Contributor

@ptoscano ptoscano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! The deprecation notices show in almost all the places, and --consumed needs to be deprecated as well (so please handle that too).

I checked what we do already to deprecate command line options; it seems that

  • in the --help option: a message such as "Deprecated, this option will be removed from the future major releases. " is prepended (yes) to the "help" text
  • in the man page: the text "Deprecated, this option will be removed in future major releases. " is prepended to the description of the option

So, as example how to apply them for the options here, I'll show what to do for the --available option:

  • in the cli:
          self.parser.add_argument(
              "--available",
              action="store_true",
              help=_("deprecated, this option will be removed from the future major releases; "
                     "show those subscriptions which are available"),
          )
  • in the man page:
    .TP
    .B --all
    Deprecated, this option will be removed in future major releases. 
    Lists all possible subscriptions that have been purchased, even if they don't match the architecture of the system. This is used with the
    .B --available
    option.

Since all the changes in the Python part will require reformatting, don't forget to use the black tool to reformat the code.

Let me know for more questions/doubts/etc.

* Options --available, --all, --ondate, --servicelevel --no-overlap
--match-install, --pool-only, --afterdate were marked as deprecated
in ListCommand definition of their help string.

* The options were marked as deprecated in list command manuald page.

CARD ID: cct-868
Copy link
Contributor

@ptoscano ptoscano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Will wait for QE verification before merging.

Copy link
Contributor

@zpetrace zpetrace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from the QE POV and the pre-verification PASSED, therefore, PR can be merged:)

@ptoscano ptoscano merged commit 9b03485 into candlepin:subscription-manager-1.29 Nov 7, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants